Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use String instead of StaticString to be more inline with swift-log behaviour #118

Merged
merged 1 commit into from
Apr 8, 2022
Merged

Use String instead of StaticString to be more inline with swift-log behaviour #118

merged 1 commit into from
Apr 8, 2022
+3 −3

Conversation

antranapp
Copy link
Contributor

I am wrapping DiagnosticsLogger inside a LogHandler from swift-log package so that I can use swift-log as the main logger for my app.

But currently, Diagnostics is using StaticString for #file, #line, #function in the log method

swift-log is using String instead of StaticString for #file, #line, #function

I think, changing StaticString to String would make it more interoperable with swift-log and provides more flexibility to integrate with other libraries as well.

@wetransferplatform
Copy link
Collaborator

wetransferplatform commented Mar 28, 2022 •
edited
Loading

Messages
📖

View more details on Bitrise
📖 DiagnosticsTests: Executed 30 tests (0 failed, 0 retried, 0 skipped) in 0.511 seconds

Code Coverage Report

Name Coverage
Diagnostics 75.23% ⚠️

Generated by 🚫 Danger Swift against a6c1075

BasThomas
BasThomas reviewed Mar 28, 2022
View reviewed changes
Copy link

@BasThomas BasThomas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution, @antranapp! I'm inclined to agree with this change, but I do wonder why swift-log chose to go this route. Do you know more about that? Or a concrete example where we'd want to "override" a file or function reference?

@antranapp
Copy link
Contributor Author

Hi @BasThomas, I found this issue explaining why swift-log uses String instead of StaticString

For my use case, I am wrapping DiagnosticsLogger inside a LogHandler provided by swift-log: https://github.com/antranapp/DebugPane_Diagnostics/blob/master/Sources/DebugPane_Diagnostics/DiagnosticsLogHandler.swift

The LogHandler protocol requires the parameters to be String instead of StaticString

With this, I can automatically get all the log that I handle globally inside my app.

@BasThomas
Copy link

Awesome, thanks for that!

I wonder if we should treat this as a breaking change — if any packages depend on this one, and expect a StaticString, that may be breaking. Would it be possible in any way to have these as overloads until we'd cut a version 4.0?

AvdLee
AvdLee approved these changes Apr 7, 2022
View reviewed changes
Copy link
Contributor

@AvdLee AvdLee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has been requested before! #108 introduced it, but it's not a necessary requirement. I guess we can directly change it back to String and just release as 4.0.0 release since it's indeed breaking.

BasThomas
BasThomas approved these changes Apr 7, 2022
View reviewed changes
Copy link

@BasThomas BasThomas left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

4.0 it is then 😊

@AvdLee
Copy link
Contributor

AvdLee commented Apr 7, 2022

I'll have a look at CI to make this PR green in the upcoming days!

@AvdLee AvdLee mentioned this pull request Apr 7, 2022
Closed
@AvdLee AvdLee merged commit 10dbfe9 into WeTransfer:master Apr 8, 2022
@wetransferplatform
Copy link
Collaborator

Congratulations! 🎉 This was released as part of Release 4.0.0 🚀

Generated by GitBuddy

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AvdLee AvdLee AvdLee approved these changes

@BasThomas BasThomas BasThomas approved these changes

@amirdew amirdew Awaiting requested review from amirdew

@kairadiagne kairadiagne Awaiting requested review from kairadiagne

@raphkoebraam raphkoebraam Awaiting requested review from raphkoebraam

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@antranapp @wetransferplatform @BasThomas @AvdLee